Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor shielded sync #3006

Closed
wants to merge 29 commits into from
Closed

Refactor shielded sync #3006

wants to merge 29 commits into from

Conversation

batconjurer
Copy link
Member

@batconjurer batconjurer commented Apr 4, 2024 •
edited
Loading

Describe your changes

This PR refactors shielded sync to make the following improvements

  • Allow fetching new masp txs and trial-decrypting notes to happen asynchronously
  • Parallelize the trial-decryptions
  • Modularize the logic so that we can mock parts of the algorithm for tests and to enable migration over to using a specila masp indexer
  • Added test coverage
  • Decouple nullifying notes and updating spent notes from the trial-decryption process
  • Refactor the masp.rs module in the sdk into several smaller files and submodules

The new architecture of the algorithm can be found here

N.B. The integration tests seem flaky on the CI, although I can't reproduce any of it locally. It's unclear to me if it is due to the changes in this PR.

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

@batconjurer batconjurer marked this pull request as draft April 4, 2024 15:42
@codecov Codecov
Copy link

codecov bot commented Apr 10, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 46.65658% with 1763 lines in your changes missing coverage. Please review.

Project coverage is 54.73%. Comparing base (883bd0f) to head (bdb61c3).

Files Patch % Lines
crates/sdk/src/masp/shielded_ctx.rs 51.29% 844 Missing ⚠️
crates/sdk/src/masp/mod.rs 29.79% 436 Missing ⚠️
crates/sdk/src/masp/utils.rs 51.28% 208 Missing ⚠️
crates/apps_lib/src/client/masp.rs 0.00% 186 Missing ⚠️
crates/sdk/src/masp/types.rs 72.61% 43 Missing ⚠️
crates/node/src/bench_utils.rs 0.00% 27 Missing ⚠️
crates/sdk/src/masp/test_utils.rs 89.79% 15 Missing ⚠️
crates/apps_lib/src/cli/client.rs 0.00% 2 Missing ⚠️
crates/node/src/lib.rs 0.00% 1 Missing ⚠️
crates/sdk/src/tx.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3006      +/-   ##
==========================================
+ Coverage   54.05%   54.73%   +0.67%     
==========================================
  Files         315      319       +4     
  Lines      106296   107702    +1406     
==========================================
+ Hits        57461    58952    +1491     
+ Misses      48835    48750      -85

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@batconjurer batconjurer force-pushed the bat/fix/improve-ss-fetching branch 2 times, most recently from 33f5189 to 08a3b7f Compare April 30, 2024 08:45
@batconjurer batconjurer marked this pull request as ready for review May 2, 2024 11:59
@batconjurer batconjurer requested review from grarco and murisi May 2, 2024 11:59
@batconjurer batconjurer changed the title Made fetching and decrypting maps notes operate in parallel. Fetching… Refactor shielded sync May 2, 2024
@batconjurer batconjurer force-pushed the bat/fix/improve-ss-fetching branch from 174c5d2 to 3bcaee8 Compare May 2, 2024 16:04
grarco
grarco previously approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@grarco grarco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, just need to investigate the failing tests

@brentstone
Copy link
Collaborator

What's the status of this? Can we get it on a 0.35.1 base? @batconjurer @grarco

@grarco
Copy link
Contributor

grarco commented May 9, 2024

I believe we haven't figured out yet the issue with the tests in CI

@brentstone brentstone mentioned this pull request May 13, 2024
Closed
@tzemanovic tzemanovic mentioned this pull request May 21, 2024
Closed
@brentstone brentstone mentioned this pull request May 24, 2024
Closed
@sug0 sug0 force-pushed the bat/fix/improve-ss-fetching branch 6 times, most recently from 1ceece0 to ca44b7b Compare May 29, 2024 14:41
@brentstone brentstone mentioned this pull request May 31, 2024
Closed
batconjurer and others added 24 commits June 3, 2024 18:32
@batconjurer
Added first unit test for shielded sync
dbb8dbf
@batconjurer
Rebasing
b024060
@batconjurer
Fixed unit test for retrying fetch and fixed the bugs it caught
c2c07be
@batconjurer
More unit tests
514f6f0
@batconjurer
Cleanup and docstrings
358e689
@batconjurer
Fixing issues from rebasing
ccb0f09
@batconjurer
Fix integration tests
dc1bd9e
@batconjurer
Fixed the check-crates
51ba26d
@batconjurer
Removed some unwraps on mutexes
af0d687
@batconjurer
reverting to unwrapping locks
6ff3646
@batconjurer
Testing something in ci
3f1f1d5
@batconjurer
Testing something in ci
1a2dc53
@batconjurer
changelog
115ef33
@batconjurer
[chore]: rebasing
829d514
@batconjurer
Fixing conflicts from rebasing
890c486
@sug0 @batconjurer
Update Cargo.lock
babfe49
@sug0 @batconjurer
Fix pruning of unscanned txs
d7d3e1d
@sug0 @batconjurer
Fix subtract underflow in stdout drawer
24351a3
@sug0 @batconjurer
Fix percent sign
a32de55
@sug0 @batconjurer
Refactor shielded utils
baf9507
@sug0 @batconjurer
Cache scanned masp txs more effectively
8183fbb
@batconjurer
[chore]: Added tests to ensure that trial decryption caching is actua…
1d5e740 
…lly used
@batconjurer
Fixed a clone implementation on tempdir in benchmarks
1539e09
@batconjurer
rebasing
5bc4268
@brentstone brentstone mentioned this pull request Jun 6, 2024
Closed
@sug0
Copy link
Contributor

sug0 commented Jul 9, 2024

Closed in favor of #3385

@sug0 sug0 closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grarco grarco grarco left review comments

@murisi murisi Awaiting requested review from murisi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@batconjurer @brentstone @grarco @sug0